test: unit tests for LocalLoginComponent#5235
Conversation
|
/request-review @Yicong-Huang |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5235 +/- ##
============================================
- Coverage 49.11% 46.18% -2.93%
+ Complexity 2378 2217 -161
============================================
Files 1051 1052 +1
Lines 40342 40734 +392
Branches 4277 4340 +63
============================================
- Hits 19812 18813 -999
- Misses 19373 20779 +1406
+ Partials 1157 1142 -15
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Xiao-zhen-Liu Please review this PR. |
Xiao-zhen-Liu
left a comment
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Overall this is a clean, well-scoped addition — thanks for the thorough coverage. A few notes:
Strengths
- Good decomposition: one component spec per PR fits the ongoing test-backfill series and keeps the review small (single new file, no production changes).
- Very readable: clear
describe/itgrouping, descriptive names, and thecreateComponent(queryParams)helper withresetTestingModule()cleanly handles thereturnUrlvariant. - The 22 cases faithfully mirror the component's behavior (validators, default-user prefill, trimming, and the success/error/fallback paths for both
loginandregister).
Main thing to address
- The
try/catcharoundcomponent.login()/component.register()in the four error-path tests is dead code, and the accompanying comment is inaccurate — see the inline note.component.login()does not throw synchronously; the synchronousnotificationService.error(...)assertions already cover the behavior, and the re-thrown error is reported asynchronously by RxJS rather than surfaced to the caller.
Minor / optional
- Prefer
mockReturnValueOnceon the already-created spy over(mock.x as any) = vi.fn()(inline note). - Coverage gaps worth a line each if you want completeness: the
this.allForms && ...guard branch inconfirmationValidator(the falsy-allFormspath) is untested, andregister()callsUserService.validateUsernamebefore the password length/match checks — no test pins that ordering, so a future reorder wouldn't be caught. beforeEachcallscreateComponent()and thereturnUrltest calls it again (each withresetTestingModule); harmless but slightly wasteful.
None of these block; the only real cleanup is the dead try/catch.
| try { | ||
| component.login(); | ||
| } catch { | ||
| // The component re-throws the error after notifying; swallow it here. | ||
| } |
There was a problem hiding this comment.
This try/catch is dead code, and the comment is inaccurate. component.login() does not throw synchronously: on error the chain hits catchError, which calls notificationService.error(...) synchronously and then returns throwError(() => e). Because the component's .subscribe() has no error callback, RxJS does not surface that re-thrown error to the caller — it reports it asynchronously via hostReportError (a setTimeout), so there is nothing here for try/catch to catch. The notificationService.error assertion just below is what actually validates the behavior.
Suggest dropping the try/catch and the comment entirely. (If you want to be defensive about the leaked async error, the cleaner fix is to give the component's .subscribe() an explicit error handler rather than relying on the global unhandled-error path.)
Same pattern recurs at L260, L343, and L362.
|
|
||
| it("surfaces the error's message via NotificationService.error on failure", () => { | ||
| vi.spyOn(UserService, "validateUsername").mockReturnValue({ result: true, message: "ok" }); | ||
| (userServiceMock.login as any) = vi.fn().mockReturnValue(throwError(() => new Error("boom"))); |
There was a problem hiding this comment.
Reassigning the mock through as any after it was already created in createComponent works but reads awkwardly and drops the typing. Since login is already a vi.fn(), prefer overriding the queued return on the existing spy:
vi.mocked(userServiceMock.login!).mockReturnValueOnce(throwError(() => new Error("boom")));Keeps the type and avoids swapping the function reference. Same at L257, L336, L355.
4 participants
What changes were proposed in this PR?
local-login.component.spec.tsto cover form construction, validators, default-user prefill, and the login/register flows that were previously untested.allFormsexposes the five expected controls withrequired,minLength(6), and the customconfirmationValidator, thatconfirmationValidatorreturns{ confirm: true }on mismatch and{}on match, and thatupdateConfirmValidatorschedulesupdateValueAndValidityon the confirmation control viasetTimeout.ngOnInitpatchesloginUsername/loginPasswordonly whenGuiConfigService.env.defaultLocalUseris populated, thatloginshort-circuits vialoginErrorMessageon validation failure and otherwise callsUserService.loginwith the trimmed username and navigates toqueryParams.returnUrlorDASHBOARD_USER_WORKFLOW, and that error paths surface the error's message (or the fallback"Incorrect username or password") throughNotificationService.error.registerenforces password length, password match, andUserService.validateUsername, callsUserService.registerwith the trimmed username on success and surfaces the account-created notification, and on error notifies with the error's message (or the fallback"Registration failed").Any related issues, documentation, or discussions?
Closes: #5226
How was this PR tested?
yarn test --include="src/app/hub/component/about/local-login/local-login.component.spec.ts", 22 tests passing.yarn format:fix, 506 files unchanged.Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.7 in compliance with ASF